Add DeepMAsED for deep learning evaluation of metagenomic de novo assemblies#1051
Add DeepMAsED for deep learning evaluation of metagenomic de novo assemblies#1051SkyLexS wants to merge 37 commits into
Conversation
Release 5.4.2
|
Warning Newer version of the nf-core template is available. Your pipeline is using an old version of the nf-core template: 3.5.2. For more documentation on how to update your pipeline, please see the nf-core documentation and Synchronisation documentation. |
jfy133
changed the title
jfy133
left a comment
jfy133
left a comment
There was a problem hiding this comment.
Initial pass, but I leave the proper review to @dialvarezs :)
… the nftignore because deepmased uses Deep Learning TensorFlow and when you run predictions for the same input the output could be of by one decimal and fail the test
|
Still have test various failures due to variation in the snapshot... |
There was a problem hiding this comment.
Hi @SkyLexS! Sorry for the delay.
Your PR looks clean, I have just a few comments. Some will need an nf-core module update, not just pipeline code.
Additionally, I would prefer to make this an opt-in feature rather than opt-out. We already run ALE by default, DeepMAsED seems to be noticeably heavier (the default CI profile went from ~30 min to ~1 h), and I don't think most users will make good use of contig misassembly scores anyway. We can leave it enabled on only one testing profile to trigger it in ci, assembly_input seems a good option for this.
Another concern: --skip_deepmased_features is redundant, per your validation, skipping features forces skipping predict, which equals --skip_deepmased. Only "features only" is a real sub-case, but is that case actually useful? If not, then we should only keep a single flag to control the execution of the tool.
I reverted the out-of-scope changes, looks like they came from trying to stabilize snapshots and/or merging the newer changes on dev, but they weren't necessary.
There's a single GTDB-Tk file failing the checksum, I will look into that to see if we can just ignore it or needs a fix.
EDIT: All snapshots are stable now.
| } | ||
|
|
||
| withName: DEEPMASED_FEATURES { | ||
| ext.args = { params.deepmased_features_gzip ? '--gzip' : '' } |
There was a problem hiding this comment.
Assuming --gzip appends .gz to the feats files, enabling this flag will make DEEPMASED_FEATURES fail, because the module only declares path("*_feats.tsv"), so Nextflow won't find the output. Supporting the flag needs an update to the nf-core module to match the compressed extension (*_feats.tsv{,.gz}).
| skip_deepmased = false | ||
| skip_deepmased_features = false | ||
| skip_deepmased_predict = false | ||
| deepmased_cpu_only = true |
There was a problem hiding this comment.
I'm not a fan of flags turned on by default. Let's make it deepmased_use_gpu instead.
|
|
||
| if (!params.skip_deepmased && !params.skip_binning) { | ||
| ch_shortread_assemblies_for_deepmased = ch_assemblies.filter { meta, _assembly -> | ||
| meta.sr_platform != null && meta.sr_platform != [] |
There was a problem hiding this comment.
The filter ch_assemblies.filter { meta -> meta.sr_platform != null && meta.sr_platform != [] } selects by read presence, not assembler. In hybrid samples the Flye/metaMDBG assemblies also carry sr_platform, so they pass the filter and get joined with their long-read BAM, then DeepMAsED ends up running on a long-read alignment, likely unintended.
It also keeps SPAdesHybrid. The output.md note says hybrid isn't supported. To be honest, I'm not an expert on these tools, but we already run ALE on hybrid. Either way, pick the channel by assembler like ALE:
- Hybrid OK (matches ALE): use
ch_shortread_assemblies(keeps MEGAHIT/SPAdes/SPAdesHybrid, drops Flye/metaMDBG) + fix the output.md line. - Exclude hybrid: filter
ch_shortread_assembliesto meta.assembler in ['MEGAHIT','SPAdes'], keep docs.
| @@ -0,0 +1,38 @@ | |||
| process DEEPMASED_PREDICT { | |||
| tag "$meta.id" | |||
| label 'process_medium' | |||
There was a problem hiding this comment.
To reliably request GPUs (especially on HPC/cloud executors), this module needs the process_gpu label. That wires it to the accelerator directive under -profile gpu.
| ================================================================================ | ||
| */ | ||
|
|
||
| if (!params.skip_binning || params.ancient_dna || !params.skip_ale) { |
There was a problem hiding this comment.
Need to add deepmased here too, because if not, enabling skip_binning will skip it silently.
|
|
||
| - `Assembly/[assembler]/QC/[sample/group]/DeepMAsED/features/` | ||
| - `[sample]-[assembler]_feature_file_paths.tsv`: Index file listing all generated feature table files | ||
| - `[sample]-[assembler]*_feats.tsv`: Per-contig feature tables (one per parallel processing bin) |
There was a problem hiding this comment.
Should have {,.gz} to consider deepmased_features_gzip.
4 participants
PR checklist
nf-core pipelines lint).nextflow run . -profile test,docker --outdir <OUTDIR>).nextflow run . -profile debug,test,docker --outdir <OUTDIR>).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).